Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 14, 2025

User description

The AddCompositionEventHandler method was registering the factory function itself in the DI container instead of invoking it, causing GetRequiredKeyedService<THandler> to fail when resolving handlers created with AddEventHandlerWithRetries.

Changes

  • SubscriptionBuilder.cs: Wrap factory function in lambda to invoke it during service resolution

    // Before: registers Func<IServiceProvider, THandler>
    Services.TryAddKeyedSingleton(SubscriptionId, getInnerHandler);
    
    // After: registers THandler by invoking the factory
    Services.TryAddKeyedSingleton(SubscriptionId, (sp, _) => getInnerHandler(sp));
  • CompositionHandlerTests.cs: Add test coverage for factory-based handler registration

The fix aligns with the existing pattern in AddEventHandler (line 48) and enables the following usage:

builder.AddEventHandlerWithRetries(
    sp => ActivatorUtilities.CreateInstance<MyHandler>(sp, sp.GetRequiredKeyedService<IService>(key)),
    retryPolicy
)
Original prompt

This section details on the original issue you should resolve

<issue_title>AddEventHandlerWithRetries with the getHandler parameter does not work</issue_title>
<issue_description>I have the following subscription and an event handler into which I need to inject a command service that works with a specific shard of KurrentDB

var subscriptionId = $"Ledger.StarterDailyAccountOpener.AccountCluster.{location.ClusterIndex}";

builder.Services
    .AddKeyedSingleton(subscriptionId, (serviceProvider, _) => serviceProvider.GetRequiredKeyedService<EventStoreClient>(shardKey))
    .AddSubscription<StreamSubscription, StreamSubscriptionOptions>(subscriptionId, subscription => subscription
        .Configure(options =>
        {
            options.StreamName = new StreamName("$et-V1.AccountCreated");
            options.ResolveLinkTos = true;
            options.ThrowOnError = true;
        })
        .AddEventHandlerWithRetries(serviceProvider => ActivatorUtilities.CreateInstance<StarterDailyAccountOpener>(
            serviceProvider,
            serviceProvider.GetRequiredKeyedService<ICommandService<DailyAccountState>>(shardKey)), _defaultResiliencePolicy));

When starting the host, I get an exception that the StarterDailyAccountOpener handler is not registered with the subscriptionId key. The problem is in the AddCompositionEventHandler method, specifically here

Services.TryAddKeyedSingleton(SubscriptionId, getInnerHandler);

In the container, not an instance of the handler is registered, but a function to obtain the handler. It should be

Services.TryAddKeyedSingleton(SubscriptionId, (serviceProvider, _) => getInnerHandler(serviceProvider))
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.


PR Type

Bug fix, Enhancement


Description

  • Fix AddCompositionEventHandler to properly invoke handler factory functions during DI resolution

    • Wrap factory in lambda to invoke it when service is resolved, not register the factory itself
  • Add comprehensive test coverage for factory-based handler registration with composition handlers

  • Refactor code formatting and modernize syntax across multiple files

    • Convert traditional constructors to primary constructors in ServiceBusMessageBuilder
    • Modernize pattern matching in ServiceBusHelper using or patterns
    • Clean up test data formatting in IsSerialisableByServiceBus
  • Add XML documentation comments to public methods in SubscriptionBuilder


Diagram Walkthrough

flowchart LR
  A["AddCompositionEventHandler<br/>with factory function"] -->|Before: registers Func| B["DI Container<br/>stores factory"]
  B -->|GetRequiredKeyedService| C["❌ Fails to resolve<br/>THandler instance"]
  A -->|After: wraps in lambda| D["DI Container<br/>invokes factory"]
  D -->|GetRequiredKeyedService| E["✓ Returns THandler<br/>instance"]
Loading

File Walkthrough

Relevant files
Bug fix
SubscriptionBuilder.cs
Fix factory invocation and add handler overload with docs

src/Core/src/Eventuous.Subscriptions/Registrations/SubscriptionBuilder.cs

  • Fix AddCompositionEventHandler overload with Func
    THandler> parameter to wrap factory in lambda (sp, _) =>
    getInnerHandler(sp) instead of registering factory directly
  • Add new overload of AddCompositionEventHandler that accepts
    Func for wrapping
    handlers with service provider access
  • Add comprehensive XML documentation comments to all public methods
    explaining parameters and behavior
  • Improve code formatting and readability
+57/-3   
Tests
CompositionHandlerTests.cs
Add composition handler factory registration tests             

src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs

  • Add new test class CompositionHandlerTests with comprehensive test
    coverage for factory-based handler registration
  • Test ShouldResolveCompositionHandlerWithFactory validates that
    handlers are correctly resolved from DI container with factory
    functions
  • Test ShouldHandleEventWithCompositionHandler validates end-to-end
    event handling through composition wrapper
  • Include test infrastructure with TestHandler, CompositionWrapper,
    TestDependency, and TestHandlerLogger helper classes
+126/-0 
Formatting
ServiceBusMessageBuilder.cs
Convert to primary constructor syntax                                       

src/Azure/src/Eventuous.Azure.ServiceBus/Producers/ServiceBusMessageBuilder.cs

  • Convert traditional constructor to primary constructor syntax for
    cleaner code
  • Replace all field references with parameter names throughout the class
  • Maintain all existing functionality while improving code readability
+17/-31 
ServiceBusHelper.cs
Modernize pattern matching and add headers                             

src/Azure/src/Eventuous.Azure.ServiceBus/Shared/ServiceBusHelper.cs

  • Modernize type checking logic using or pattern matching instead of
    nested ternary operators
  • Add copyright and license header comments
  • Improve code readability and maintainability
+24/-27 
IsSerialisableByServiceBus.cs
Clean up test data formatting                                                       

src/Azure/test/Eventuous.Tests.Azure.ServiceBus/IsSerialisableByServiceBus.cs

  • Remove unnecessary using System.Threading.Tasks import
  • Clean up test data formatting with consistent alignment and spacing
  • Remove redundant inline comments for primitive type literals
+17/-18 
Miscellaneous
EsdbEventStore.cs
Add unused variable in ReadEvents                                               

src/EventStore/src/Eventuous.EventStore/EsdbEventStore.cs

  • Add unused variable x in ReadEvents method (appears to be incomplete
    or debug code)
  • Minor formatting adjustment with extra spacing
+2/-1     

…ction

Co-authored-by: alexeyzimarev <2821205+alexeyzimarev@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix AddEventHandlerWithRetries not working with getHandler parameter Fix AddCompositionEventHandler DI registration with factory functions Nov 14, 2025
Copilot AI requested a review from alexeyzimarev November 14, 2025 10:20
@github-actions
Copy link

Test Results

 51 files  + 34   51 suites  +34   29m 54s ⏱️ + 19m 41s
264 tests + 12  264 ✅ + 12  0 💤 ±0  0 ❌ ±0 
795 runs  +532  795 ✅ +532  0 💤 ±0  0 ❌ ±0 

Results for commit 6295dc5. ± Comparison against base commit a5bb50c.

This pull request removes 5 and adds 17 tests. Note that renamed tests count towards both.
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/12/2025 1:14:01 PM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/12/2025 1:14:01 PM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(4e788d76-39cd-4b58-a178-c0b712758899)
Eventuous.Tests.Subscriptions.SequenceTests ‑ ShouldReturnFirstBefore(CommitPosition { Position: 0, Sequence: 1, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 4, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 6, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-12T13:14:01.4847774+00:00 })
Eventuous.Tests.Subscriptions.SequenceTests ‑ ShouldReturnFirstBefore(CommitPosition { Position: 0, Sequence: 1, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 6, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 8, Timestamp: 2025-11-12T13:14:01.4847774+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-12T13:14:01.4847774+00:00 })
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:02:30 AM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:02:30 AM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:02:37 AM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:02:37 AM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:02:44 AM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:02:44 AM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(13e46484-7f7e-43c7-ab92-a1e4483abbe4)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(26c63cf8-f597-430e-903b-1eecb2676ab4)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(413bf1e8-7f31-4923-8fcd-edaf39caa5eb)
Eventuous.Tests.Subscriptions.CompositionHandlerTests ‑ ShouldHandleEventWithCompositionHandler
…

@alexeyzimarev alexeyzimarev marked this pull request as ready for review November 14, 2025 11:09
@alexeyzimarev alexeyzimarev merged commit 1f77727 into dev Nov 14, 2025
5 checks passed
@alexeyzimarev alexeyzimarev deleted the copilot/fix-event-handler-retry-issue branch November 14, 2025 11:10
@qodo-free-for-open-source-projects
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #450
🟢 Fix the AddCompositionEventHandler method to properly register handler instances in the DI
container instead of registering the factory function itself
Change the registration from Services.TryAddKeyedSingleton(SubscriptionId,
getInnerHandler) to Services.TryAddKeyedSingleton(SubscriptionId, (serviceProvider, _) =>
getInnerHandler(serviceProvider))
Enable AddEventHandlerWithRetries to work correctly with the getHandler parameter when
using factory functions
Allow injection of keyed services (like ICommandService) into event handlers created via
ActivatorUtilities.CreateInstance
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- CompositionHandlerTests
- Startup
- ConfigureServices
- TestSub
- TestHandler
Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Meaningless variable name: Variable x is declared with a meaningless single-letter name and assigned true without
clear purpose or usage

Referred Code
bool x              = true;

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #450
🟢 Fix the AddCompositionEventHandler method to properly register handler instances in the DI
container instead of registering the factory function itself
Change the registration from Services.TryAddKeyedSingleton(SubscriptionId,
getInnerHandler) to Services.TryAddKeyedSingleton(SubscriptionId, (serviceProvider, _) =>
getInnerHandler(serviceProvider))
Enable AddEventHandlerWithRetries to work correctly with the getHandler parameter by
ensuring handlers can be resolved via GetRequiredKeyedService
Allow injection of command services with specific shards (keyed services) into event
handlers created via factory functions
Codebase Duplication Compliance
🟢
No codebase code duplication found New Components Detected (Top 5):
- CompositionHandlerTests
- Startup
- ConfigureServices
- TestSub
- TestHandler
Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Meaningless variable name: Variable x is declared with a meaningless single-letter name and assigned true without
clear purpose or usage

Referred Code
bool x              = true;

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Validate the composed handler correctly

Modify the ShouldResolveCompositionHandlerWithFactory test to validate that the
correct composed handler (CompositionWrapper) is registered for the
subscription, instead of only checking the inner handler (TestHandler).

src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs [38-45]

 [Test]
 public async Task ShouldResolveCompositionHandlerWithFactory() {
     // This test validates that AddCompositionEventHandler correctly registers
     // handlers when using a factory function
-    var handler = _server.Services.GetRequiredKeyedService<TestHandler>("sub-with-factory");
-    await Assert.That(handler).IsNotNull();
-    await Assert.That(handler.Dependency.Value).IsEqualTo("test-value");
+    var subs = _server.Services.GetServices<TestSub>().ToArray();
+    var sub  = subs.FirstOrDefault(x => x.SubscriptionId == "sub-with-factory");
+    sub.ShouldNotBeNull();
+
+    var handlers = sub.ResolveHandlers(_server.Services);
+    await Assert.That(handlers.Length).IsEqualTo(1);
+    var handler = handlers[0];
+    await Assert.That(handler).IsInstanceOfType(typeof(CompositionWrapper));
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the test ShouldResolveCompositionHandlerWithFactory only validates the inner handler's registration, not the composed handler. While another test covers the composed handler's execution, improving this test to validate the composition directly would make the test suite more robust and explicit.

Medium
High-level
Remove unrelated changes from PR

The PR contains unrelated refactoring and stylistic changes in Azure and
EventStoreDB components. These should be removed to keep the PR focused solely
on the intended bug fix for DI registration.

Examples:

src/Azure/src/Eventuous.Azure.ServiceBus/Producers/ServiceBusMessageBuilder.cs [11-17]
class ServiceBusMessageBuilder(
        IEventSerializer                serializer,
        string                          streamName,
        ServiceBusMessageAttributeNames attributes,
        ServiceBusProduceOptions?       options                = null,
        Action<string>?                 setActivityMessageType = null
    ) {
src/EventStore/src/Eventuous.EventStore/EsdbEventStore.cs [127]
                    bool x              = true;

Solution Walkthrough:

Before:

// In src/Azure/.../ServiceBusMessageBuilder.cs
class ServiceBusMessageBuilder(
        IEventSerializer                serializer,
        string                          streamName,
        ...
    ) {
    // ... class refactored to use primary constructor
}

// In src/EventStore/.../EsdbEventStore.cs
public async Task<StreamEvent[]> ReadEvents(...) {
    ...
    var  resolvedEvents = await read.ToArrayAsync(cancellationToken).NoContext();
    bool x              = true; // Unused variable
    return ToStreamEvents(resolvedEvents);
    ...
}

After:

// The following files should not be included in this PR,
// as their changes are unrelated to the main bug fix:
// - src/Azure/src/Eventuous.Azure.ServiceBus/Producers/ServiceBusMessageBuilder.cs
// - src/Azure/src/Eventuous.Azure.ServiceBus/Shared/ServiceBusHelper.cs
// - src/Azure/test/Eventuous.Tests.Azure.ServiceBus/IsSerialisableByServiceBus.cs
// - src/EventStore/src/Eventuous.EventStore/EsdbEventStore.cs

// The PR should only contain changes related to the bug fix in:
// - src/Core/src/Eventuous.Subscriptions/Registrations/SubscriptionBuilder.cs
// - src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies multiple unrelated changes, including refactoring and a debugging leftover, which reduces the PR's clarity and focus, although it doesn't address a functional bug in the intended fix.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AddEventHandlerWithRetries with the getHandler parameter does not work

2 participants